Align TROP lambda conventions with paper (Athey et al. 2025) by igerber · Pull Request #119 · igerber/diff-diff

igerber · 2026-01-31T22:58:41Z

Summary

Fix disabled-parameter semantics for λ_time and λ_unit to match the paper's convention: 0.0 = uniform weights (exp(-0×dist)=1), not ∞
Add ValueError validation rejecting inf in lambda_time_grid and lambda_unit_grid
Remove dead ∞→0.0 conversions for time/unit lambdas in Python (4 locations) and Rust (5 locations)
Fix Stage 1 LOOCV: λ_nn search now fixes lambda_time=0.0 (was ∞)
Update Registry equations to match Section 2.2 and Eq. 2-3 of the paper
Add special cases section (recovers DID/TWFE, Matrix Completion, SC/SDID)
Mark jackknife variance as implementation addition (paper describes only bootstrap)

Methodology references (required if estimator / math changes)

Method name(s): TROP (Triply Robust Panel Estimator)
Paper / source link(s): Athey, S., Imbens, G.W., Qu, Z., & Viviano, D. (2025). Triply Robust Panel Estimators. arXiv:2508.21536v2. Section 2.2, Equations 2-3, Section 4.3, Table 5, Footnote 2.
Any intentional deviations from the source (and why): None — this PR removes deviations. Previously λ_time=∞ and λ_unit=∞ were accepted and converted to 0.0 internally. The paper uses λ=0 directly for uniform weights. Only λ_nn=∞ (factor model disabled) is a valid infinity input.

Validation

Tests added/updated: tests/test_trop.py — 4 tests updated to use 0.0 instead of inf for time/unit grids, 1 new test for ValueError on inf in time/unit grids
All 84 TROP tests pass on both Python and Rust backends
Rust backend rebuilt with maturin develop --release

Security / privacy

Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

Fix disabled-parameter semantics for λ_time and λ_unit to match the paper's convention: 0.0 means uniform weights (exp(-0×dist)=1), not ∞. Only λ_nn=∞ remains valid (disables factor model). Add ValueError validation rejecting inf in time/unit grids, remove dead ∞→0 conversions in Python and Rust, fix Stage 1 LOOCV fixed values, and update Registry equations to match Section 2.2 and Eq. 2-3 of the paper. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

github-actions · 2026-01-31T23:03:50Z

Overall assessment: ⚠️ Needs changes

Executive summary

Affected method: TROP; Python side now enforces λ_time/λ_unit=0 for uniform weights per registry and updates tests.
Rust backend still accepts inf in time/unit grids without validation, which can produce degenerate weights and violates registry semantics if the Rust API is used directly or invalid grids slip through.
Rust docstrings still say inf disables time/unit weights; update to avoid misleading users.

Methodology

Severity: P1 — Impact: Rust loocv_grid_search / loocv_grid_search_joint accept inf in lambda_time_grid/lambda_unit_grid and apply it directly, yielding exp(-inf×dist)=0 rather than uniform weights and contradicting registry + Python validation; can bias estimates or cause failures in Rust backend callers. rust/src/trop.rs:L318-L382, rust/src/trop.rs:L1460-L1516, docs/methodology/REGISTRY.md:L537-L542, diff_diff/trop.py:L498-L511. — Fix: add explicit inf checks in Rust entrypoints (grid search + bootstrap) to raise Python ValueError when time/unit grids contain inf; mirror Python validation and registry.

Code Quality

No findings.

Performance

No findings.

Maintainability

No findings.

Tech Debt

No findings.

Security

No findings.

Documentation/Tests

Severity: P3 — Impact: Rust docstrings still advertise fixed_time/fixed_unit as inf disabled, which is now incorrect and can mislead callers. rust/src/trop.rs:L150-L163. — Fix: update comments to 0.0 semantics and note only λ_nn=∞ is valid; add a Rust-side test if you implement the validation above.

Defense-in-depth: validate lambda_time_grid and lambda_unit_grid don't contain inf in loocv_grid_search and loocv_grid_search_joint Rust entrypoints, matching the Python-side validation. Fix univariate_loocv_search docstring to use paper conventions (0.0 for uniform weights, inf only for disabling factor model). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

igerber · 2026-01-31T23:32:20Z

/ai-review

github-actions · 2026-01-31T23:37:01Z

🔁 AI review rerun (requested by @igerber)

Head SHA: b350d659d4d391fb6942075be01e7a3238074e3b

Overall assessment: ⚠️ Needs changes

Executive summary

TROP λ_time/λ_unit semantics now align with the Methodology Registry (0.0 = uniform weights, inf invalid; λ_nn=∞ disables factor model), and the Python/Rust LOOCV paths are consistent.
Validation for inf in time/unit grids plus updated tests is good coverage for the new API behavior.
One user-facing docstring still claims λ_time/λ_unit=∞ are accepted and converted, which now contradicts the constructor validation.

Methodology

No findings. TROP λ semantics and LOOCV stage‑1 settings match the Registry (Eq. 3/Footnote 2) in both Python and Rust. (See diff_diff/trop.py:L493-L1542, rust/src/trop.rs:L342-L405, docs/methodology/REGISTRY.md:L521-L566.)

Code Quality

No findings.

Performance

No findings.

Maintainability

No findings.

Tech Debt

No findings.

Security

No findings.

Documentation/Tests

Severity: P2 | Impact: TROP.fit docstring still states “λ_time/λ_unit=∞ → 0.0 (uniform weights)” even though the constructor now raises on inf for those grids, which can mislead users and conflicts with the Methodology Registry. | Fix: Update the docstring to state that lambda_time_grid/lambda_unit_grid must be finite, 0.0 means uniform weights, and only λ_nn accepts ∞ (converted to 1e10). Location: diff_diff/trop.py:L1873-L1880.

The Returns docstring still claimed λ_time/λ_unit=∞ were converted to 0.0 internally. This contradicts the constructor validation added in this PR which now raises on inf for those grids. Updated to accurately state that inf is not accepted for λ_time/λ_unit while λ_nn=∞ is still converted to 1e10. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

igerber · 2026-01-31T23:44:41Z

/ai-review

github-actions · 2026-01-31T23:52:27Z

🔁 AI review rerun (requested by @igerber)

Head SHA: 5e7f571e0a9558c7c7e960add3661aee2e14144c

Overall assessment: ✅ Looks good

Executive summary

TROP (twostep + joint) lambda_time/lambda_unit semantics now use 0.0=uniform with inf reserved for lambda_nn, aligned with the Methodology Registry.
LOOCV Stage-1 fixed-parameter settings are updated in Python and Rust and variance paths still convert lambda_nn only; no methodology mismatches found.
Minor robustness gap: grid validation only happens in __init__, so mutated/unpickled instances could bypass the new “no inf” contract (P3).

Methodology

No findings; lambda_time/lambda_unit disabled semantics and Stage-1 LOOCV fixed-parameter setup match the registry updates. (diff_diff/trop.py:2058, rust/src/trop.rs:369, docs/methodology/REGISTRY.md:533)

Code Quality

Severity: P3 | Impact: The new “no inf in lambda_time/lambda_unit grids” contract is only enforced in __init__; unpickled or post-init mutated instances can bypass it and, if Rust LOOCV errors are swallowed, the Python fallback will silently use exp(-inf*dist)=0 weights, diverging from the documented error behavior. | Concrete fix: Add a shared grid validation at the start of fit() (or before LOOCV in _fit_twostep/_fit_joint) and re-raise invalid-grid errors instead of falling back. (diff_diff/trop.py:506, diff_diff/trop.py:2043, diff_diff/trop.py:1411)

Performance

No findings.

Maintainability

No findings.

Tech Debt

No findings.

Security

No findings.

Documentation/Tests

No findings; tests and registry text were updated for the new lambda semantics and validation. (tests/test_trop.py:2124, docs/methodology/REGISTRY.md:533)

igerber merged commit ee2dc1c into main Feb 1, 2026
7 checks passed

igerber deleted the fix/trop-lambda-paper-convention branch February 1, 2026 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align TROP lambda conventions with paper (Athey et al. 2025)#119

Align TROP lambda conventions with paper (Athey et al. 2025)#119
igerber merged 3 commits intomainfrom
fix/trop-lambda-paper-convention

igerber commented Jan 31, 2026

Uh oh!

github-actions bot commented Jan 31, 2026

Uh oh!

igerber commented Jan 31, 2026

Uh oh!

github-actions bot commented Jan 31, 2026

Uh oh!

igerber commented Jan 31, 2026

Uh oh!

github-actions bot commented Jan 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

igerber commented Jan 31, 2026

Summary

Methodology references (required if estimator / math changes)

Validation

Security / privacy

Uh oh!

github-actions bot commented Jan 31, 2026

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

igerber commented Jan 31, 2026

Uh oh!

github-actions bot commented Jan 31, 2026

Overall assessment: ⚠️ Needs changes

Executive summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

igerber commented Jan 31, 2026

Uh oh!

github-actions bot commented Jan 31, 2026

Overall assessment: ✅ Looks good

Executive summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant